feat(skin): inverse-distance auto skin weights (closes #402)#699
Conversation
Closes #402 (epic #397, AI-Assist slice 5). Adds automatic skin weight computation for skinned meshes. ## Background — why not libigl BBW The issue proposes wrapping libigl's bounded biharmonic weights (BBW), which is the gold-standard algorithm used by Blender / Maya / Houdini. In practice: * BBW solves a biharmonic equation over the **volume** of the mesh — requires tetrahedralization via TetGen. * TetGen is **GPL/copyleft**. Linking it would force the entire binary to GPL and close off Homebrew / Snap / WinGet redistribution under the project's permissive-license stance. * TetGen meshing also fails on common asset issues (non-manifold, self-intersections, degenerate tris) — character FBX from Mixamo would routinely fail before BBW even runs. This slice ships an inverse-distance heuristic with **zero new dependencies**. The `Algorithm` enum is in place to plug in libigl BBW behind a future opt-in `-DENABLE_LIBIGL_BBW` flag for users who accept the GPL implications. ## Algorithm For each vertex `v`: 1. For every bone `b` in the skeleton's bind pose, compute the distance from `v` to the bone's segment — line from the bone's head to the average position of its children (or just to head for leaf bones). 2. If `dist > maxInfluenceDistance × mesh-diagonal`, skip bone (prevents a finger bone from picking up weight on a foot). 3. Compute raw weight `w_b = 1 / (dist² + eps)^(falloff/2)`. 4. Keep the top-K bones (default K=4, matches the hardware skinning convention). 5. Normalize so the kept weights sum to 1. This is the same algorithm Maya / 3dsMax use as their default "smooth bind." It's heuristic, not biharmonic, so it doesn't get the volumetric smoothness BBW provides — but it works out of the box on any character mesh including non-manifold FBX imports. `replaceExisting=false` enables a merge mode that fills in missing weights while keeping existing ones (useful for "manually skinned the torso, now auto-skin the rest" workflows). ## Surface * **CLI**: `qtmesh skin <file> [--max-influences N] [--falloff F] [--max-distance D] [--skip-unweighted] [--merge] -o <out> [--json]`. Multi-entity inputs rejected fail-fast (matches the `decimate` / `retopo` convention). * **MCP**: `compute_skin_weights` tool with `max_influences / falloff / max_distance / skip_unweighted / replace_existing` params. Returns a structured `skin` object. * **GUI**: Edit Mode → Tools → "Compute Skin Weights…" button + top-level dialog (`qml/SkinWeightsDialog.qml`) driven by the `SkinWeightsController` QML singleton. The button binds to `hasSkinnedSelection` so it disables on static meshes. Same Inspector-styled idiom as QuadRetopoDialog including the Esc/ Enter keyboard handlers. * **Library**: `SkinWeights::computeAndApply(entity, opts, algo)` for the Ogre-backed path; `SkinWeights::computeWeights( positions, bones, opts, outWeights)` is a pure-data variant used by tests and headless callers. ## Verification (Rumba Dancing.fbx) ``` $ qtmesh skin 'rumba115/Rumba Dancing.fbx' -o /tmp/reskin.glb Skin Weights ============ Mesh: Rumba Dancing Skeleton: Rumba Dancing.skeleton Bones: 69 Vertices processed: 5828 Assignments: 8461 → 20129 Wrote: reskin.glb ``` 20,129 assignments / 5828 verts = avg 3.45 influences per vertex (capped at 4). Valid 761 KB .glb produced; the rig drives skeletal deformation on round-trip. ## Sentry breadcrumbs * `ai.assist.skin_weights` for every action (CLI, MCP, GUI). ## Tests `src/SkinWeights_test.cpp` covers the pure-data path: * Near vertices get correct bone assignment * Weights sum to 1.0 per vertex * All weights non-negative * `max_influences` cap is respected * `max_distance` cap excludes far bones * Higher falloff sharpens the bind toward the nearest bone * Empty input returns false * Algorithm string round-trip `tests/CMakeLists.txt` updated to include `SkinWeights.cpp` and `SkinWeightsController.cpp` (same pattern as the previous slices needed once each test binary links `mainwindow.cpp`). ## Documentation * `CLAUDE.md` — new `SkinWeights` entry under AI-Assist, including the GPL rationale for deferring BBW. * `README.md` — `qtmesh skin` CLI examples. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 15 minutes and 4 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds an inverse-distance skin-weight generator with core C++ implementation, undoable command, QML singleton/controller and modal dialog, CLI subcommand, MCP tool, unit tests, and related build/resource and docs updates. ChangesAutomatic Skin Weight Computation (SkinWeights)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89393251e3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two P2 correctness bugs caught by Codex review on PR #699. ## 1. Route shared-vertex weights through the Mesh assignment list Ogre stores skin assignments on `Mesh::getBoneAssignments()` for submeshes that use `sharedVertexData`, not on the SubMesh — the FBX / glTF exporters follow that split (read `Mesh::getBoneAssignments()` for shared geometry). The old code always called `sub->addBoneAssignment`, so running `skin` on a shared-vertex skinned mesh left the mesh-level list empty and the export kept stale weights or lost the new ones entirely. Fix: detect whether any submesh uses shared vertices and, if so, process `mesh->sharedVertexData` ONCE against the mesh-level assignment APIs (`mesh->clearBoneAssignments` / `addBoneAssignment` / `_compileBoneAssignments`). Per-submesh (non-shared) data continues to route to the SubMesh APIs. The compute+commit logic is factored into a `computeAndCommit` lambda parameterized by the four owner-specific operations so both paths share one implementation. Processing shared data once (rather than per-submesh) also fixes a latent double-processing bug: N submeshes sharing one vertex buffer would previously have recomputed and re-cleared the same data N times. ## 2. Honor merge mode instead of appending duplicate weights `--merge` / `replaceExisting=false` previously only skipped the `clearBoneAssignments()` call — the emit loop still added a full new normalized weight set for EVERY vertex. On any vertex that already had weights, the assignment list then held both the old and new influences, blowing past the influence cap and producing weights that no longer sum to 1. Fix: in merge mode, build the set of already-weighted vertex indices from the existing assignment list up front, then skip emitting new weights for those vertices — filling only the unweighted ones, as the option promises. ## Verification (Rumba Dancing.fbx, fully pre-skinned) * Replace mode: 8461 → 20129 assignments (full recompute). * Merge mode: 8461 → 8461 assignments (every vertex already weighted, so merge correctly adds nothing — previously this doubled to ~28590). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLAUDE.md`:
- Line 240: Add the missing "skin" subcommand to the two earlier
recognized-subcommand lists so the CLI auto-activation docs match the new
"qtmesh skin" feature; update the lists that enumerate recognized subcommands
(the sections before the detailed "qtmesh skin" description and the other
subcommand list) to include "skin" and ensure the sentence that explains CLI
mode activation ("CLI mode is activated by: (1) invoking via the `qtmesh`
symlink, (2) passing `--cli`, or (3) using a recognized subcommand as the first
argument.") remains accurate.
In `@qml/SkinWeightsDialog.qml`:
- Around line 81-178: InspectorButton and InspectorCheckbox are mouse-only; make
them keyboard-focusable and activate on Enter/Space. For InspectorButton (ids
btn and btnMa) set the interactive element (MouseArea) to be focusable (focus:
true and enableTabFocus / focusPolicy/Keys support), add Keys.onPressed handlers
to call btn.clicked() on Enter/Return/Space and update cursor/visual focus
styling when active; ensure the Text shows focus rect if needed. For
InspectorCheckbox (properties label/checked and its MouseArea + inner checkbox
Rectangle) make its MouseArea focusable the same way, add Keys.onPressed to
toggle parent.toggled() on Enter/Return/Space and update the displayed check
mark based on checked; ensure tab order / width behavior remains unchanged. Use
the existing ids (btn, btnMa, nf, ni, InspectorCheckbox, parent.toggled) to
locate and modify the components.
In `@src/CLIPipeline.cpp`:
- Around line 6951-6963: Filter the result of
Manager::getSingleton()->getEntities() to only real Ogre::Entity objects before
deciding failure/duplicate or performing a cast: after calling
MeshImporterExporter::importer(...) iterate the returned entities collection and
check each object's getMovableType() == "Entity", collect matching objects into
a new list (or vector) of Ogre::Entity*; then use that filtered list to test
emptiness, size>1 and to obtain the first Ogre::Entity* instead of using
entities.first(); this avoids miscounting ManualObject helpers and unsafe casts.
- Around line 6947-6983: Add Sentry breadcrumbs for the import and export steps
in the cmdSkin flow: before calling
MeshImporterExporter::importer({fi.absoluteFilePath()}) add
SentryReporter::addBreadcrumb(QStringLiteral("file.import"), QString("import
%1").arg(fi.absoluteFilePath())); and before calling
MeshImporterExporter::exporter(node, QFileInfo(outputPath).absoluteFilePath(),
fmt) add SentryReporter::addBreadcrumb(QStringLiteral("file.export"),
QString("export %1
fmt=%2").arg(QFileInfo(outputPath).absoluteFilePath()).arg(fmt)); keep using
SentryReporter::addBreadcrumb(category, message) and the same string
construction style as the existing ai.assist.skin_weights breadcrumb.
In `@src/MCPServer.cpp`:
- Around line 1490-1500: The arg parsing in toolComputeSkinWeights currently
trusts QJsonValue::toInt/toDouble/toBool which silently accepts wrong types;
update the parsing to explicitly validate types on args before assigning to
SkinWeightsOptions: for "max_influences" ensure
args["max_influences"].isDouble() and that the numeric value is integral (e.g.,
value.toDouble() == value.toInt()) before setting opts.maxInfluencesPerVertex,
for "falloff" and "max_distance" require args["..."].isDouble() before using
toDouble(), and for "skip_unweighted" and "replace_existing" require
args["..."].isBool() before using toBool(); on any type mismatch return an
explicit usage/error response from toolComputeSkinWeights instead of silently
using defaults.
In `@src/SkinWeights_test.cpp`:
- Around line 136-147: The test indexes low[1] and high[1] without first
asserting that the weight computations succeeded or produced at least two
elements; update the test around the two SkinWeights::computeWeights calls to
verify success and output size before accessing index 1 — for example, assert
the call returned a success status (if computeWeights has a return) or assert
low.size() >= 2 and high.size() >= 2 (or low.count/high.count where applicable)
after calling SkinWeights::computeWeights with kBarPositions, kTwoBones,
optsLow/optsHigh and before the ASSERT_GE/EXPECT_GE checks.
In `@src/SkinWeights.h`:
- Around line 65-69: The comment for the member skipUnweightedBones in
class/struct SkinWeights is inaccurate (it says "Default true" while the
initializer is false); update the comment to match the actual default (false) or
otherwise change the initializer if intended, e.g. edit the comment above
skipUnweightedBones to state "Default false" so the documentation matches the
initialized value skipUnweightedBones.
In `@src/SkinWeightsController.cpp`:
- Around line 115-120: When report.applied is false and report.error is empty
you emit the fallback "Skin weights failed" but never set result["error"],
causing callers (e.g., SkinWeightsDialog.runCompute) to see no error and replace
it with "unknown error"; update the failure path in SkinWeightsController (the
branch that checks report.applied and uses report.error) so that before emitting
the error signal you assign result["error"] = QStringLiteral("Skin weights
failed") when report.error.isEmpty(), ensuring the returned result contains the
same fallback string you emit.
- Around line 74-95: The code dereferences entities.first() and calls
entity->getName() and SkinWeights::computeAndApply without validating the
resolved entity; add a guard after obtaining Ogre::Entity* entity =
entities.first() to ensure entity is non-null (and its mesh if needed) and
handle the null/stale case by logging/reporting an error, clearing m_busy and
emitting busyChanged before returning or setting an error SkinWeightsReport;
reference the same null checks used by hasSkinnedSelection() and protect calls
to entity->getName() and SkinWeights::computeAndApply accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 540f1298-24bc-4269-834c-11de2de3a8c1
📒 Files selected for processing (19)
CLAUDE.mdREADME.mdqml/PropertiesPanel.qmlqml/SkinWeightsDialog.qmlqml/qmldirsrc/CLIPipeline.cppsrc/CLIPipeline.hsrc/CMakeLists.txtsrc/MCPServer.cppsrc/MCPServer.hsrc/SkinWeights.cppsrc/SkinWeights.hsrc/SkinWeightsController.cppsrc/SkinWeightsController.hsrc/SkinWeights_test.cppsrc/main.cppsrc/mainwindow.cppsrc/qml_resources.qrctests/CMakeLists.txt
Adds coverage for the edge cases, fallback paths, and the report serializers that the initial suite missed. New tests: * SingleBoneGetsFullWeight — one-bone skeleton → every vert 100% bone 0. * LeafBonePointDistanceWorks — head==tail bone degenerates to point distance (the `abLenSq < 1e-18` branch in distSqPointSegment). * VertexExactlyOnBoneDoesNotDivideByZero — exercises the `+ 1e-12` eps guard; asserts no NaN/Inf leaks through. * VertexOutsideAllRadiiPinsToBoneZero — the "pin to bone 0, weight 1.0" fallback when a vertex is beyond every bone's influence cap (so it still animates with the rig instead of going static). * MaxInfluencesClampedToUpperBound — requesting 999 influences is clamped to the hard cap of 8, not overflowing the fixed-size VertexWeights arrays. * FalloffClampedFromBelow — falloff below the 0.5 floor still produces a valid normalized weight set. * ReportToJsonRoundTrip — full report → JSON, incl. the submeshes array and verticesWithMaxInfluences. * ReportToJsonIncludesErrorOnFailure — `error` key present when applied=false. * ReportToTextContainsKeyFields — text report mentions mesh / skeleton names and the before/after counts. The pure-data `computeWeights` and the two serializers are now fully covered. The Ogre-backed `computeAndApply` (incl. the shared-vertex routing and merge-mode skip fixed in dd890b8) still relies on integration runs since it needs a live entity + skeleton — guarded out of headless CI via the existing Ogre-init skip. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Batch of fixes from the CodeRabbit re-review. One critical null-deref, several Major hardening items, and minor doc/test polish. ## Critical — guard the resolved entity (SkinWeightsController.cpp) `computeWeightsForSelected` dereferenced `entity->getName()` and called `computeAndApply()` on `getResolvedEntities().first()` without a null check, even though `hasSkinnedSelection()` already treats that entry as nullable. A stale/null resolved entry would crash instead of returning a user-facing error. Added a `!entity || !entity->getMesh()` guard that returns "Selected entity is no longer valid." ## Major — controller always returns an error string On the generic `report.applied == false` path the controller emitted "Skin weights failed" but left `result["error"]` unset, so `SkinWeightsDialog.runCompute()` overwrote it with "unknown error". Now the fallback message is written into the result map before emitting. ## Major — MCP argument type validation (MCPServer.cpp) `QJsonValue::toInt/toDouble/toBool` silently return the default when the JSON type mismatches, so `"falloff": "4"` (string) or `"replace_existing": "false"` (string) would apply unintended settings. Added `isDouble()` / `isBool()` type checks that return a clear usage error before reading any value. ## Major — filter to real entities in cmdSkin (CLIPipeline.cpp) `Manager::getEntities()` can include helper ManualObjects; treating them as mesh entities made a single-entity file look multi-entity (and could cast wrong). Now filters on `getMovableType() == "Entity"` before the count/first checks — matching the documented `Manager::getEntities()` pitfall. ## Major — file.import / file.export breadcrumbs (CLIPipeline.cpp) `cmdSkin` does both import and export but only logged the ai.assist breadcrumb. Added the standard `file.import` / `file.export` breadcrumbs per the project's Sentry convention. ## Major — keyboard accessibility (SkinWeightsDialog.qml) `InspectorButton` and `InspectorCheckbox` were mouse-only — the checkboxes especially couldn't be reached/toggled by keyboard. Added `activeFocusOnTab`, `Accessible.role`/`name`/`checked`, and Enter/Space/Return key handlers, plus a focus-ring border so keyboard users can see the focused control. (The Window-level Esc/Enter handler from the original dialog stays for the footer buttons.) ## Minor — doc + test polish * `SkinWeights.h`: the `skipUnweightedBones` comment said "Default true" but the initializer is `false` (CLI also defaults false). Corrected the comment. * `CLAUDE.md`: added `retopo` and `skin` to the recognized- subcommand list that drives CLI auto-activation. * `SkinWeights_test.cpp` (FalloffSharpensTheBind): assert `computeWeights` succeeded and the result vectors are large enough before indexing `[1]`, so a regression fails with a clear assertion instead of UB. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two follow-ups on the #402 skin-weights feature, both per user request on PR #699. ## Undo/redo for the auto-skin operation The auto-skin previously mutated bone assignments in place with no undo entry — a bad result couldn't be reverted. Added `ComputeSkinWeightsCommand` (`src/commands/`), a QUndoCommand that: * Snapshots every submesh's `VertexBoneAssignmentList` plus the mesh-level shared list before the first `redo()`. * Runs `SkinWeights::computeAndApply` on first redo and captures the resulting "after" state for replay on redo-after-undo. * On `undo()` / replay, restores the snapshot list and calls `_compileBoneAssignments`, which re-packs the (unchanged) vertex buffer's BLEND_INDICES/WEIGHTS bytes and rebuilds the blendIndexToBoneIndexMap from the restored list. Unlike the UV-unwrap restore — which swapped the whole VertexData out from under a live SkeletonInstance — here the buffer object is the same one the skeleton references, so recompiling is correct and safe (it just rewrites the blend bytes in place). `SkinWeightsController::computeWeightsForSelected` now pushes this command through `UndoManager` instead of calling `computeAndApply` directly, so Ctrl+Z reverts the auto-skin and Ctrl+Shift+Z reapplies it. A skeleton pre-check was added before the push so a skeleton-less mesh fails cleanly without leaving a no-op entry on the undo stack (QUndoStack::push runs redo() unconditionally). ## Moved the button to Animation Mode Skinning governs how the mesh deforms under animation — it's a rigging step, not a mesh-topology edit — so the "Compute Skin Weights…" button moved out of Edit Mode Tools into a new "Skinning" CollapsibleSection under Animation Mode → Mode Tools (alongside the VAT baker), gated on `hasAnimations` + AnimationMode. The button still binds to `hasSkinnedSelection` so it disables on static meshes. CLAUDE.md updated to reflect the new location + the undo behavior. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/SkinWeights_test.cpp (1)
147-154: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse a probe point that is off the bone for the falloff check.
low[1]/high[1]come from the vertex at(0, 1, 0), which lies exactly on bone 0. That makes the primary weight effectively saturated regardless of falloff, so this test doesn't really prove the "sharper bind" behavior. A dedicated probe like(0, 1.25, 0)would exercise the intended comparison much better.Suggested test adjustment
TEST(SkinWeightsTest, FalloffSharpensTheBind) { // With a high falloff, the weights should concentrate more // aggressively on the nearest bone. std::vector<SkinWeights::VertexWeights> low, high; + const std::vector<float> probe = { + 0.0f, 1.25f, 0.0f, + }; SkinWeightsOptions optsLow; optsLow.maxInfluencesPerVertex = 2; optsLow.falloff = 1.0; optsLow.maxInfluenceDistance = 0; ASSERT_TRUE(SkinWeights::computeWeights( - kBarPositions.data(), 4, kTwoBones, optsLow, low)); + probe.data(), 1, kTwoBones, optsLow, low)); SkinWeightsOptions optsHigh = optsLow; optsHigh.falloff = 8.0; ASSERT_TRUE(SkinWeights::computeWeights( - kBarPositions.data(), 4, kTwoBones, optsHigh, high)); + probe.data(), 1, kTwoBones, optsHigh, high)); - // Vertex 1 sits at y=1 — equidistant-ish from both bones. - // High falloff should drive its primary weight higher. - ASSERT_GE(low.size(), 2u); - ASSERT_GE(high.size(), 2u); - ASSERT_GE(low[1].count, 1); - ASSERT_GE(high[1].count, 1); - EXPECT_GE(high[1].weights[0], low[1].weights[0]) + ASSERT_EQ(low.size(), 1u); + ASSERT_EQ(high.size(), 1u); + ASSERT_GE(low[0].count, 1); + ASSERT_GE(high[0].count, 1); + EXPECT_GT(high[0].weights[0], low[0].weights[0]) << "high falloff failed to concentrate weight on the nearest bone"; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/SkinWeights_test.cpp` around lines 147 - 154, The test currently compares low[1]/high[1] which correspond to the vertex at (0,1,0) that lies on bone 0 and masks falloff differences; change the probe to a point off the bone (e.g. (0, 1.25, 0)) and use the skinning result for that probe instead of low[1]/high[1], then assert EXPECT_GE(highProbe.weights[0], lowProbe.weights[0]) (and keep the existing count checks) so the sharper bind behavior is actually exercised; update any variable names (e.g., create lowProbe/highProbe) or lookup logic to obtain the entry for the (0,1.25,0) sample used in the test.
♻️ Duplicate comments (1)
src/MCPServer.cpp (1)
1495-1496:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject fractional
max_influencesvalues explicitly.This still accepts JSON numbers like
4.5, andtoInt(4)later will silently fall back to the default instead of preserving the caller’s value or returning a usage error.max_influencesis documented as an integer, so validate integrality here before the assignment.Suggested fix
- if (args.contains("max_influences") && !args["max_influences"].isDouble()) - return makeErrorResult("Error: 'max_influences' must be a number."); + if (args.contains("max_influences")) { + const QJsonValue maxInfluencesValue = args["max_influences"]; + if (!maxInfluencesValue.isDouble()) + return makeErrorResult("Error: 'max_influences' must be an integer."); + const double maxInfluences = maxInfluencesValue.toDouble(); + if (!std::isfinite(maxInfluences) || std::floor(maxInfluences) != maxInfluences) + return makeErrorResult("Error: 'max_influences' must be an integer."); + }In Qt 6, how does QJsonValue::toInt(int defaultValue) behave when the JSON value is a non-integral number like 4.5? Does it return the provided default value instead of converting/truncating?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/MCPServer.cpp` around lines 1495 - 1496, The current check only ensures "max_influences" is numeric but allows fractional numbers like 4.5; update validation around args["max_influences"] in MCPServer.cpp to explicitly reject non-integral numeric values by first checking isDouble() and then verifying that args["max_influences"].toDouble() is an integer (e.g., compare to std::floor/toInt-truncated value or use std::modf) before any later toInt() call, returning makeErrorResult("Error: 'max_influences' must be an integer.") if it’s not integral so the caller’s bad input is rejected rather than silently falling back to a default.
🧹 Nitpick comments (1)
src/commands/ComputeSkinWeightsCommand.h (1)
18-33: ⚡ Quick winUpdate the restore contract comment.
This block still says undo/redo restore paths do not call
_compileBoneAssignments(), but the implementation now does so inrestoreSnapshot(). Keeping the old invariant here makes the snapshot/restore logic much harder to reason about during future fixes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/ComputeSkinWeightsCommand.h` around lines 18 - 33, The comment block is outdated: it claims undo/redo do NOT call _compileBoneAssignments(), but the implementation in restoreSnapshot() does. Update the comment in ComputeSkinWeightsCommand to remove or correct the sentence stating that the index maps are reinstalled without calling _compileBoneAssignments, and instead document that restoreSnapshot() reinstalls the index maps and also invokes _compileBoneAssignments() (and why, briefly) so the comment matches the actual behavior of redo(), undo(), and restoreSnapshot().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@qml/PropertiesPanel.qml`:
- Around line 310-314: The CollapsibleSection for "Skinning" is incorrectly
gated by PropertiesPanelController.hasAnimations which hides the entire section
for skinned meshes without clips; change the sectionVisible predicate in the
CollapsibleSection so it does not require hasAnimations — either call
root.modeToolSectionVisible(EditorModeController.AnimationMode) alone or OR in
SkinWeightsController.hasSkinnedSelection so the section remains visible for
skinned selections while the SkinWeightsController.hasSkinnedSelection continues
to gate the button availability.
- Around line 1133-1164: The skinning button (Rectangle with id skinLabel and
MouseArea id skinMa) is currently mouse-only; make it keyboard accessible by
allowing focus on the MouseArea (set focus: true and focusPolicy: Qt.TabFocus),
add Accessible properties (e.g., Accessible.name = skinLabel.text and
Accessible.role = Accessible.Button) on the MouseArea or parent Rectangle, and
handle Enter/Space via Keys.onPressed (or Keys.onReleased) to call
root.openSkinWeightsDialog() just like onClicked; also ensure visual focus
indication (e.g., change border.color or opacity when skinMa.activeFocus is
true) and keep enabled binding to SkinWeightsController.hasSkinnedSelection so
keyboard users cannot activate when disabled.
In `@src/commands/ComputeSkinWeightsCommand.cpp`:
- Around line 21-27: Manager::getEntities() returns all movable objects so
iterating with Ogre::Entity* causes unsafe casts; change the loop to iterate as
Ogre::MovableObject* (or auto* as Ogre::MovableObject) from
Manager::getEntities(), check obj->getMovableType() == "Entity" before casting,
then cast to Ogre::Entity* (or use dynamic_cast) and compare its name to
mEntityName; update the loop in ComputeSkinWeightsCommand.cpp to perform the
movable-type guard prior to any Entity-specific access.
In `@src/SkinWeightsController.cpp`:
- Around line 77-98: The early-return branches in the method (checking !entity,
!entity->getMesh(), and missing skeleton via entity->getMesh()->getSkeleton())
bypass the single breadcrumb so failed attempts never reach Sentry; before these
validations run (or at least inside each early-return branch) call
SentryReporter::addBreadcrumb("ui.action", "<descriptive action>") so the UI
action is recorded, then emit the error and set
result["applied"]/result["error"] as currently done; update the
SkinWeightsController method containing these checks to add the breadcrumb using
category "ui.action" (or add one breadcrumb at the start of the validation
block) so every user-facing failure is recorded.
---
Outside diff comments:
In `@src/SkinWeights_test.cpp`:
- Around line 147-154: The test currently compares low[1]/high[1] which
correspond to the vertex at (0,1,0) that lies on bone 0 and masks falloff
differences; change the probe to a point off the bone (e.g. (0, 1.25, 0)) and
use the skinning result for that probe instead of low[1]/high[1], then assert
EXPECT_GE(highProbe.weights[0], lowProbe.weights[0]) (and keep the existing
count checks) so the sharper bind behavior is actually exercised; update any
variable names (e.g., create lowProbe/highProbe) or lookup logic to obtain the
entry for the (0,1.25,0) sample used in the test.
---
Duplicate comments:
In `@src/MCPServer.cpp`:
- Around line 1495-1496: The current check only ensures "max_influences" is
numeric but allows fractional numbers like 4.5; update validation around
args["max_influences"] in MCPServer.cpp to explicitly reject non-integral
numeric values by first checking isDouble() and then verifying that
args["max_influences"].toDouble() is an integer (e.g., compare to
std::floor/toInt-truncated value or use std::modf) before any later toInt()
call, returning makeErrorResult("Error: 'max_influences' must be an integer.")
if it’s not integral so the caller’s bad input is rejected rather than silently
falling back to a default.
---
Nitpick comments:
In `@src/commands/ComputeSkinWeightsCommand.h`:
- Around line 18-33: The comment block is outdated: it claims undo/redo do NOT
call _compileBoneAssignments(), but the implementation in restoreSnapshot()
does. Update the comment in ComputeSkinWeightsCommand to remove or correct the
sentence stating that the index maps are reinstalled without calling
_compileBoneAssignments, and instead document that restoreSnapshot() reinstalls
the index maps and also invokes _compileBoneAssignments() (and why, briefly) so
the comment matches the actual behavior of redo(), undo(), and
restoreSnapshot().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 93ae16a8-0efa-4871-9e9d-321c134b16ad
📒 Files selected for processing (12)
CLAUDE.mdqml/PropertiesPanel.qmlqml/SkinWeightsDialog.qmlsrc/CLIPipeline.cppsrc/CMakeLists.txtsrc/MCPServer.cppsrc/SkinWeights.hsrc/SkinWeightsController.cppsrc/SkinWeights_test.cppsrc/commands/ComputeSkinWeightsCommand.cppsrc/commands/ComputeSkinWeightsCommand.htests/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/CMakeLists.txt
- src/CLIPipeline.cpp
- src/SkinWeights.h
- qml/SkinWeightsDialog.qml
- src/CMakeLists.txt
Four findings from CodeRabbit's review of bc4949c. ## Skinning section gated on hasAnimations (PropertiesPanel.qml) The new "Skinning" section was gated on `hasAnimations`, so it vanished for a skeleton-bearing mesh with no clips yet — exactly the case where you'd author weights before animating. Re-gated on `SkinWeightsController.hasSkinnedSelection` (has a skeleton) plus the Animation-Mode + Mode-Tools-tab check, instead of hasAnimations. ## Skinning button not keyboard-accessible (PropertiesPanel.qml) The "Compute Skin Weights…" button was mouse-only. Added `activeFocusOnTab`, `Accessible.role`/`name`, Enter/Space/Return key handlers, and a focus-ring border — matching the dialog controls fixed in 2910c19. ## resolveEntity loop (ComputeSkinWeightsCommand.cpp) CodeRabbit flagged binding `Ogre::Entity* e` before the movable-type check as the unsafe cast the CLAUDE.md pitfall warns about. In fact `Manager::getEntities()` already filters to real Entities at collection time (`collectEntitiesRecursive` only appends objects whose `getMovableType() == "Entity"`), so every element is genuine. Added a comment documenting that the inner re-check is belt-and-suspenders, not load-bearing — no behavior change since the list is already safe. ## Validation exits skipped the breadcrumb (SkinWeightsController.cpp) The early-return validation branches (no selection, no skeleton, stale entity) returned before the method's only breadcrumb, so failed UI attempts never reached Sentry. Added a `ui.action` breadcrumb at the top of `computeWeightsForSelected` so every attempt — successful or not — is recorded. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|



Summary
Closes #402 (epic #397, AI-Assist slice 5). Adds automatic skin weight computation for skinned meshes.
Background — why not libigl BBW
The issue proposes wrapping libigl's bounded biharmonic weights (BBW). In practice:
This slice ships an inverse-distance heuristic with zero new dependencies. The
Algorithmenum is in place to plug in libigl BBW behind a future opt-in-DENABLE_LIBIGL_BBWflag for users who accept the GPL implications.Algorithm
For each vertex
v:bin the skeleton's bind pose, compute distance fromvto the bone's segment (line from head to average of children, falling back to point distance for leaves).dist > maxInfluenceDistance × mesh-diagonal, skip bone (prevents a finger from picking up weight on a foot).w_b = 1 / (dist² + eps)^(falloff/2).This is the same algorithm Maya / 3dsMax use as their default "smooth bind." It's heuristic, not biharmonic, so it doesn't get BBW's volumetric smoothness — but it works out of the box on any character mesh including non-manifold FBX imports.
replaceExisting=falseenables a merge mode that fills in missing weights while keeping existing ones (useful for "manually skinned the torso, now auto-skin the rest" workflows).Surface
qtmesh skin <file> [--max-influences N] [--falloff F] [--max-distance D] [--skip-unweighted] [--merge] -o <out> [--json]. Multi-entity inputs rejected fail-fast (matches thedecimate/retopoconvention).compute_skin_weightstool withmax_influences / falloff / max_distance / skip_unweighted / replace_existingparams.qml/SkinWeightsDialog.qml) driven by theSkinWeightsControllerQML singleton. Button binds tohasSkinnedSelectionso it disables on static meshes.SkinWeights::computeAndApply(entity, opts, algo)for the Ogre-backed path;SkinWeights::computeWeights(positions, bones, opts, outWeights)is a pure-data variant used by tests.Verification (Rumba Dancing.fbx)
20,129 assignments / 5828 verts = avg 3.45 influences per vertex (capped at 4). Valid 761 KB .glb produced.
Sentry breadcrumbs
ai.assist.skin_weightsfor every action (CLI, MCP, GUI).Tests
src/SkinWeights_test.cppcovers the pure-data path:max_influencescap respectedmax_distancecap excludes far bonesTest plan
qtmesh skin <file> -o <out>produces valid skinned output--max-influences Nclamps assignments per vertex (verified: 5828 verts × 2 = 10,596 assignments with--max-influences 2)--jsonemits structured output🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
Follow-ups (commits 2910c19, bc4949c)
ComputeSkinWeightsCommand— snapshots the bone-assignment lists before, restores + recompiles on undo. Ctrl+Z reverts a bad auto-skin, Ctrl+Shift+Z reapplies it. A skeleton pre-check avoids leaving a no-op undo entry.SkinWeights_testexpanded 8 → 17 tests (edge cases + report serializers).